Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use the asset getter in totalAssets, _deposit and _withdraw in ERC4626 #5322

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use the asset getter in totalAssets, _deposit and _withdraw in ERC4626 #5322

wants to merge 2 commits into from
+10 −5

Conversation

ernestognw
Copy link
Member

Fixes #5320

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 26, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 8d04788

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ernestognw
Copy link
Member Author

I'm including a changeset for this because it's technically breaking.

Let's say contract A inherits from OpenZeppelin's ERC4626 and overrode asset(). Then, after upgrading, the developer may not expect that now asset() is called. This is risky if (for example), the asset() performs a static call as it may introduce a way to execute read-only reentrancy.

I haven't come up with 1) an honest reason to insert a staticcall in the middle of asset() or 2) a way in which the contract will break after an upgrade.

Overall I think having the changelog entry should be enough

@ernestognw ernestognw requested a review from Amxx November 26, 2024 16:54
@ernestognw ernestognw mentioned this pull request Nov 26, 2024
Open
@ernestognw ernestognw changed the title Erc4626/overrideable asset Use the asset getter in totalAssets, _deposit and _withdraw in ERC4626 Nov 26, 2024
@Amxx Amxx added this to the 5.3 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amxx Amxx Awaiting requested review from Amxx

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
ignore-changeset
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.

ERC4626 replace _asset with asset() in order to be easily overrideable
2 participants
@ernestognw @Amxx